Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ICU-21964 use a single LICENSE file #2415

Merged
merged 1 commit into from
Apr 12, 2023
Merged

ICU-21964 use a single LICENSE file #2415

merged 1 commit into from
Apr 12, 2023

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Apr 10, 2023 •
edited by markusicu
Loading

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-21964
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)

c.f. unicode-org/cldr#2849

Why

  • more standard filename, visible at the top level in GitHub
  • reduce maintenance

What's inside

  • make the icu4c and icu4j LICENSE files symlinks
  • Some URLs won't work until this is merged

confirmed

  • ICU4C make dist uses git archive which copies the symlink into a file
  • ICU4J ant icu4jSrcJar makes a .jar file including LICENSE as a file.
  • Windows supports symlinks too, see

@srl295 srl295 self-assigned this Apr 10, 2023
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295 srl295 mentioned this pull request Apr 10, 2023
Merged
5 tasks
@markusicu markusicu assigned yumaoka and unassigned srl295 Apr 10, 2023
markusicu
markusicu requested changes Apr 10, 2023
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right ticket for this seems to be ICU-21964 which is assigned to @yumaoka.
Please change the commit message and PR title.
I set Yoshito as the PR assignee (=main reviewer).

markusicu
markusicu reviewed Apr 10, 2023
View reviewed changes
LICENSE Outdated Show resolved Hide resolved
docs/userguide/icu/release.md Outdated Show resolved Hide resolved
icu4j/maven-build/maven-icu4j/pom.xml Show resolved Hide resolved
@markusicu
Copy link
Member

PS: @srl295 thanks for doing this!

@srl295 srl295 changed the title ICU-22309 use a single LICENSE file ICU-21964 use a single LICENSE file Apr 10, 2023
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295
ICU-21964 use a single LICENSE file
7bf4412 
- make the icu4c and icu4j LICENSE files symlinks
- fix paths
@srl295 srl295 requested a review from markusicu April 11, 2023 19:44
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4j/maven-build/maven-icu4j/pom.xml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295
Copy link
Member Author

srl295 commented Apr 11, 2023

PTAL

markusicu
markusicu approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm tnx!

@srl295 srl295 merged commit 312bae8 into unicode-org:main Apr 12, 2023
@srl295 srl295 deleted the icu-22309/rename-license branch April 12, 2023 19:36
@yumaoka
Copy link
Member

yumaoka commented Apr 13, 2023 •
edited
Loading

@srl295 We used to include LICENSE file also in library jars (e.g. icu4j.jar). Now, with this change, LICENSE file is included in a jar, but with the content ../../../../LICENSE.

@srl295
Copy link
Member Author

srl295 commented Apr 13, 2023

@srl295 We used to include LICENSE file also in library jars (e.g. icu4j.jar). Now, with this change, LICENSE file is included in a jar, but with the content ../../../../LICENSE.

Really? I had tried it and it had the correct content.

OK, is this on Windows without symlink support?

Maybe we could have a conditional build step that uses ../../../../LICENSE directly if available otherwise the more 'local' file.

@yumaoka
Copy link
Member

yumaoka commented Apr 14, 2023

@srl295 You're right. It looks this is a problem on system not supporting symlink. I tried both Windows and Ubuntu, and only Windows output had the problem.

@srl295
Copy link
Member Author

srl295 commented Apr 14, 2023

@srl295 You're right. It looks this is a problem on system not supporting symlink. I tried both Windows and Ubuntu, and only Windows output had the problem.

Well, we could say for builds you need to enable symlinks. Or fix another way.

@yumaoka
Copy link
Member

yumaoka commented Apr 14, 2023

I'm also wondering if we need LICENSE file in each library jar file. We probably should check if we need to continue this.

@srl295
Copy link
Member Author

srl295 commented Apr 14, 2023

I'm also wondering if we need LICENSE file in each library jar file. We probably should check if we need to continue this.

It's probably a good idea. By the way common practice seems to be to put it in META-INF/LICENSE not in the root.

@markusicu
Copy link
Member

New ticket please with a problem description and maybe a proposed plan?

@srl295
Copy link
Member Author

srl295 commented Apr 14, 2023

@markusicu do we really need a new ticket or just to reopen this 74.1 ticket as still having trouble? I can do either

@markusicu
Copy link
Member

@markusicu do we really need a new ticket or just to reopen this 74.1 ticket as still having trouble? I can do either

reopening is fine

@tuffnatty
Copy link

tuffnatty commented Dec 14, 2023 •
edited
Loading

Hello @srl295,

  • ICU4C make dist uses git archive which copies the symlink into a file

It may be true, but the 74.2 src archive contains a dangling icu/LICENSE symlink pointing to nonexistant ../LICENSE; build attempts fail at make install. The 74.1 src archive is OK, though; still, I've decided to report it here as the most related ticket.

P.S. Oh. I've found there's already a ticket. Please disregard.

@srl295
Copy link
Member Author

srl295 commented Dec 14, 2023

Hello @srl295,

  • ICU4C make dist uses git archive which copies the symlink into a file

It may be true, but the 74.2 src archive contains a dangling icu/LICENSE symlink pointing to nonexistant ../LICENSE; build attempts fail at make install. The 74.1 src archive is OK, though; still, I've decided to report it here as the most related ticket.

P.S. Oh. I've found there's already a ticket. Please disregard.

Ticket is the right path. Thanks. !

@srl295
Copy link
Member Author

srl295 commented Dec 19, 2023

See #2749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusicu markusicu markusicu approved these changes

@echeran echeran Awaiting requested review from echeran

@yumaoka yumaoka Awaiting requested review from yumaoka

Assignees

@yumaoka yumaoka

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@srl295 @markusicu @yumaoka @tuffnatty